Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Swift 3 and Xcode 8 support #78

Merged
merged 32 commits into from
Sep 14, 2016
Merged

Add support for Swift 3 and Xcode 8 support #78

merged 32 commits into from
Sep 14, 2016

Conversation

drmohundro
Copy link
Owner

At the moment, SWXMLHash itself compiles, but the tests won't compile without a corresponding Swift 3 version of both Quick and Nimble.

This PR will also be what ships in SWXMLHash version 3.0.0.

The library itself compiles, but the tests are blocked by not having
Swift 3 versions of Quick and Nimble.
@drmohundro drmohundro self-assigned this Jun 19, 2016
@drmohundro
Copy link
Owner Author

drmohundro commented Jul 13, 2016

UPDATE - see below comments, this now works just fine in Swift 3.0.

I've converted over to using XCTest to get around the tests not compiling. So the good news is that the tests now compile!

...unfortunately, most of the tests are failing if not crashing entirely.

So yeah, this isn't ready to go for Swift 3.0 yet.

As NSXMLParser turned into XMLParser (without NS) there is a name clash
now and SWXMLHash’s own XMLParser must be prefixed with Foundation for
the delegate methods.
Also, they must not be private.
@thomas-em
Copy link
Contributor

thomas-em commented Jul 15, 2016

Hi David, I just created a pull request for the fixes regarding Swift3/Xcode 8 beta 2. As I just signed up today on GitHub only to share these fixes I don't have a clue what to do next and if you see that pull request at all so I thought I'd drop a comment here :).
One more thing: I shut up the warnings about unused results by assigning to _ in d1a6bcf. Maybe using @discardableResult for the stack's pop() function would be ok but I didn't want to be too intrusive.

Fixes to make SWXMLHash work with Swift 3 of current Xcode 8 beta 2 (8S162m)
@drmohundro
Copy link
Owner Author

@thomas-em awesome, thanks! I hadn't even considered the removal of the NS prefix resulting in a name collision. Merged and tests all pass! Swift 3.0 is a go!

Also, 👍 regarding silencing the compiler warnings regarding unused results - I had planned to do something about that, but I figured getting the tests passing would be top priority.

Thanks again!

@thomas-em
Copy link
Contributor

Hi David, you’re welcome. I know what you mean. I stared at the code and, misled by what the compiler told me, fiddled with it for hours until I finally had that duh!-facepalm!-moment :).

Just in case it might help anybody: only removing the private access modifier made it work (in the sense of the delegate methods being called), but the compiler warned about the methods only nearly matching the optional protocol requirements. I guess (and just verified) it »worked« because the methods don’t do anything with the parser parameter. I didn’t dig any deeper but I guess the reason why they are called even with the wrong parameter type specifier is because NSXMLParser (well, Foundation.XMLParser now) does the good old if-respondsToSelector dance and selectors in Objective C match by parameter names, not by parameter types.

Rereading my commit message for 45d5f29, I seem to have mixed up two sentences in my mind but I see that my point came across anyway :).

Regarding the unused results warning, I think using @discardableResult for pop() would be ok, as I think calling pop() just for the side effect of removing a stack’s topmost element is a common use case. Alternatively, more purist if you will, a drop() function (not returning anything) could be added. In contrast, I think having to assign to _ when using byKey() (in XMLParsingTests) is the right thing to do. If you want me to make a pull request with either of these changes I’d be happy to do so. If you don’t think it’s worth making a fuss over it, that’s fine, too. I’m happy SWXMLHash works with Swift 3 now :).

Cheers, Thomas

@drmohundro
Copy link
Owner Author

@thomas-em I went ahead and added a drop method for now. It just didn't feel right to have a stack that didn't have a pop implementation :)

The built-in `ErrorProtocol` was renamed to `Error` which conflicted
with `XMLIndexer.Error` - as a result, I've renamed it to
`IndexingError` and moved it back to the outer scope.
@drmohundro
Copy link
Owner Author

Note for anyone following, the latest commit resulted in a change to XMLIndexer.Error. This is because the built-in ErrorProtocol was renamed to Error which conflicted with XMLIndexer.Error. As a result, I've renamed it to IndexingError and moved it back to the outer scope.

@drmohundro
Copy link
Owner Author

@jpsim
Copy link
Contributor

jpsim commented Aug 19, 2016

By the way, I'm maintaining a Swift 3 branch in my personal fork which I'm keeping up to date against the latest Xcode 8 Betas as they come out, as well as supporting Linux at reasonably equivalent points as the Xcode betas.

Note that I'm not maintaining support for tests in there, all I need is the minimum to support my ongoing work to support Xcode 8 / Linux with SourceKitten: jpsim/SourceKitten#223

Feel free to copy anything you see there in here, otherwise I'll make PRs once Xcode 8 GM lands.

@drmohundro
Copy link
Owner Author

@jpsim awesome, thanks! Also, if I get the chance, I'll try to see about getting some of your changes merged over.

@jpsim
Copy link
Contributor

jpsim commented Sep 13, 2016

@drmohundro Xcode 8 is now officially out of beta 😄. Here's a diff with my branch, if you care to update this: xcode-8.0...jpsim:jp-swift-3

@drmohundro
Copy link
Owner Author

@jpsim yep, downloading it right now! (I thought I had downloaded the GM earlier...) I'm planning to get this into master tonight!

@drmohundro drmohundro merged commit 7b22cf1 into master Sep 14, 2016
@drmohundro drmohundro deleted the xcode-8.0 branch September 14, 2016 02:04
@drmohundro
Copy link
Owner Author

@jpsim FYI, I checked out your branch and, to be honest, @norio-nomura already did the work for me :) So 👍 all around!

Thanks to @jpsim, @norio-nomura, @thomas-em and @gca3020 for all of their hard work on this release! (and I'm sure I missed someone else, too...)

I just pushed 3.0.1 (the point release was because I had a fail with CocoaPods... make sure to 1) run pod spec lint first and 2) delete DerivedData if you see any old compiler errors. Sigh.

@jpsim
Copy link
Contributor

jpsim commented Sep 14, 2016

Thanks for the release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants